Add KQL string matching rules to the coding guidelines - #2223
Add KQL string matching rules to the coding guidelines#2223RolandKrummenacher wants to merge 3 commits into
Conversation
The coding guidelines covered general practice, content, and the changelog, but said nothing about KQL — the primary language of the hub — so nothing contradicted the tolower()/contains patterns that accumulated across the ingestion scripts until #2213/#2220. Add a KQL section to docs-wiki/Coding-guidelines.md covering: - Why tolower() in comparison position is always wrong (KQL string operators are already case-insensitive; _cs are the sensitive ones), with an avoid/prefer table for the common rewrites. - has vs contains as a semantic distinction (whole term vs arbitrary substring), not just a performance one, including when contains is genuinely required and the term-index limits for short/punctuation needles. - How to verify a swap: per-row cross-tabulation on real data rather than aggregate counts, plus the executable equivalence harness. Also surface the rule directly in AGENTS.md so Claude Code and Copilot see it without following the link — the file previously listed Bicep, PowerShell, markdown and commit conventions, but not KQL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
microsoft-github-policy-service
Bot
added
the
Needs: Review 👀
There was a problem hiding this comment.
Pull request overview
Documents project-specific KQL string-matching rules in the contributor coding standards so recurring anti-patterns (e.g., tolower() in comparisons and overuse of contains) are prevented via clear guidance alongside existing CI enforcement.
Changes:
- Adds a new ⚡ KQL section to
docs-wiki/Coding-guidelines.mdcovering case-insensitive operators,hasvscontains, and validation guidance. - Adds a concise KQL rule summary to
AGENTS.mdunder Coding Standards so assistants and contributors see it inline.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs-wiki/Coding-guidelines.md | Introduces detailed KQL string matching guidance (operators, has vs contains, and verification steps). |
| AGENTS.md | Adds an inline KQL standards summary referencing the coding guidelines and existing CI enforcement. |
Suppressed comments (1)
docs-wiki/Coding-guidelines.md:65
- This row states that
=~accepts a dynamic operand directly. That can be true for scalar dynamic values, but it is not universally safe if the field can be an object/array (or otherwise not string-like), and may produce confusing comparisons. Suggest qualifying the guidance to avoid implyingtostring()is never needed for dynamic fields.
| `tostring(Dyn.Field) =~ 'true'` | `Dyn.Field =~ 'true'` | These operators accept a dynamic operand directly |
Review flagged that 'these operators accept a dynamic operand directly'
was too absolute for fields that can hold an object or array.
Verified on a live cluster: tostring() is not the missing piece — raw
and tostring()-wrapped comparisons return identical results for every
payload shape, including objects and arrays. The real hazard is that a
non-scalar is compared against its JSON serialization, where has
matches a value nested anywhere in the text:
dynamic({"nested":"true"}) has 'true' -> true
dynamic({"nested":"true"}) =~ 'true' -> false
Scope the table row to scalar dynamic values and add a note describing
the JSON-serialization behavior, why tostring() does not help, and what
to use instead (extract the member, or array_index_of/set_has_element
for arrays). All claims in the note executed against a cluster.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Good catch on the dynamic-operand row — it was too absolute. Qualified in 150c52d. Verified the actual behavior on a live cluster first, and the result is a bit different from what the comment assumed:
So the real hazard isn't a missing cast — it's that a non-scalar gets compared against its JSON serialization, where The table row is now scoped to scalar dynamic values, with an IMPORTANT note covering the JSON-serialization behavior, why |
Review flagged that the indexof(Col,'x') >= 0 -> Col has 'x' row was not an equivalence: indexof() is substring-based while has is term-based, so presenting it as a mechanical rewrite could lead contributors into a behavior change. Verifying on a live cluster showed the row was wrong on a second axis too — indexof() is case-sensitive: value indexof>=0 contains has 'Windows Server' true true true 'WindowsServer' true true false <- term vs substring 'MyWindowsBuild' true true false <- term vs substring 'windows server' false true false <- case sensitivity So indexof(Col,'x') >= 0 is exactly contains_cs, not contains and not has. Reframe the row to keep the real advice (don't compute a position you only need as a boolean) while stating both differences and telling contributors to pick by intent. This mirrors the has-vs-contains distinction the section already makes, which the original row contradicted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
||
| | ❌ Avoid | ✅ Prefer | Why | | ||
| | --- | --- | --- | | ||
| | `tolower(Col) contains 'term'` | `Col has 'term'` | `has` matches whole terms using the term index; `tolower()` disables it | |
There was a problem hiding this comment.
I thought has was tokenized and case-sensitive. Is it not?
| | ❌ Avoid | ✅ Prefer | Why | | ||
| | --- | --- | --- | | ||
| | `tolower(Col) contains 'term'` | `Col has 'term'` | `has` matches whole terms using the term index; `tolower()` disables it | | ||
| | `tolower(Col) == 'value'` | `Col =~ 'value'` | `=~` is the case-insensitive equality operator | |
There was a problem hiding this comment.
nit: Does =~ do a tolower on both values? I thought tolower(Col) == '{lowercase-literal}' was actually faster. Do we know?
| | `tolower(Col) contains 'term'` | `Col has 'term'` | `has` matches whole terms using the term index; `tolower()` disables it | | ||
| | `tolower(Col) == 'value'` | `Col =~ 'value'` | `=~` is the case-insensitive equality operator | | ||
| | `tolower(a) != tolower(b)` | `a !~ b` | One comparison instead of two per-row allocations | | ||
| | `Col contains 'Windows'` | `Col has 'Windows'` | Whole-word needle – see below for when `contains` is right | |
There was a problem hiding this comment.
As far as I know, this can be a tokenization trap, aside from the case-sensitivity.
| | `indexof(Col, 'x') >= 0` | `Col has 'x'` *or* `Col contains 'x'` | Don't compute a position you don't need – but pick the operator by intent, not mechanically: `indexof()` is case-**sensitive** and substring-based, so it is equivalent to `contains_cs`, not to `has` | | ||
| | `tostring(Dyn.Field) =~ 'true'` | `Dyn.Field =~ 'true'` | These operators accept a *scalar* dynamic operand directly – see the note below | | ||
|
|
||
| Prefer `in~` over chained `=~` comparisons, and `has_any` / `has_all` over chained `has`. |
There was a problem hiding this comment.
nit: Why not work these into the table like the others for consistency?
|
|
||
| ### When `contains` is required | ||
|
|
||
| ADX tokenizes string columns at ingestion: runs of alphanumeric characters become *terms*, and punctuation (`/`, `.`, `-`, space) are separators. `has` matches whole terms and can use the term index; `contains` scans for an arbitrary substring. That makes them **semantically different**, not just faster and slower. |
There was a problem hiding this comment.
nit: I'd prefer to see this baked into the guidance above. I'd rather see 2 lines, one for each with the valid cases for each so all rules are cleanly listed together. Not a blocker, but this merged with what's above would tell the whole story. What's above alone can lead to bugs. This applies to both H3s here.
🛠️ Description
The coding guidelines cover general practice, content, and the changelog, but say nothing about KQL — the primary language of the hub.
docs-wiki/Coding-guidelines.md,AGENTS.md, andCONTRIBUTING.mddon't mention KQL or Kusto at all today. That's part of why the redundanttolower()comparisons and word-searchcontainsusages accumulated across the ingestion scripts until #2213 / #2220 — nothing in the project's written standards contradicted them.This PR writes those rules down where both humans and AI assistants will find them.
docs-wiki/Coding-guidelines.md— a new ⚡ KQL section:tolower()in comparison position is always wrong (every KQL string operator is already case-insensitive; the_csforms are the case-sensitive ones), with an avoid/prefer table for the common rewrites —tolower(x) contains→has,tolower(x) ==→=~,tolower(a) != tolower(b)→!~,indexof(...) >= 0→has, and dynamic operands not needing atostring()wrapper.containsis required: framed as a semantic distinction (whole term vs. arbitrary substring) rather than a performance one, with the legitimate cases — needles fused inside a larger token (ConsumedUnit contains 'MB'also matchingMbps), word-stem matching, and fragments that never form a whole term. Also documents the term-index limits for punctuation and sub-3-character needles.AGENTS.md— the rule is also stated inline under Coding Standards. That file already listed Bicep, PowerShell, markdown, and commit conventions but not KQL, and it's what both Claude Code (viaCLAUDE.md) and Copilot (via.github/copilot-instructions.md) load. Stating it there means an assistant sees the rule without having to follow the link.No behavior change — documentation only. The guidance is already enforced in CI by
HubsKqlOperators.Tests.ps1(from #2220); this PR documents the reasoning so contributors know why before the test tells them no.📋 Checklist
🔬 How did you test this change?
📦 Deploy to test?
🙋♀️ Do any of the following that apply?
📑 Did you update docs/changelog?
🤖 Generated with Claude Code